Refactor: make host_build_graph's includes say what each file uses - #2101
Merged
ChaoZheng109 merged 1 commit intoSep 3, 2026
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (48)
💤 Files with no reviewable changes (15)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThis change removes unused and transitive headers across common, A2A3, and A5 host build graph code. It adds direct assertion and runtime-status dependencies. Profiling-specific device-time inclusion is now conditional. ChangesHost build graph include cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This refactor makes file dependencies explicit without changing runtime behavior; the stated configurations and tests pass, and no actionable merge-blocking risk remains. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
poursoul
force-pushed
the
refactor/make-host-build-graphs-includes-say-what-each
branch
3 times, most recently
from
September 3, 2026 01:24
9681612 to
a78a501
Compare
Every translation unit and header under host_build_graph now includes what it uses and nothing else, so a reader can tell a file's dependencies from its include block rather than by following a chain of headers that happened to carry something along. 105 includes are removed and 51 added, a net 54 fewer. Three headers were acting as unofficial hubs. runtime_types.h carried runtime_status.h and aicore_completion_mailbox.h for consumers that never named them; common.h carried assert_compat.h the same way; and runtime.h carried <stdio.h> and <string.h> under comments -- "for fprintf, printf", "for memset" -- naming symbols runtime.h itself never uses. All are gone, and the files reaching SIMPLER_ERROR_*, always_assert, debug_assert and std::memset through them now include those headers directly. Same for the smaller cases: tensormap.h takes tensor_create_info.h, async_wait.h takes aicore_completion_mailbox_types.h, runtime_maker.cpp takes host_phase_kind.h, sdma_completion_scheduler.h takes constants.h and cache_maintenance.h. scheduler.h's aicpu/device_time.h moves inside the profiling conditional. Every one of its eight uses of get_sys_cnt_aicpu sits in a SIMPLER_*_PROFILING block, so an unconditional include claimed a dependency the file only has when timing is compiled in. Its comment claimed early-dispatch doorbell timing used it too, which no call site does. The same file now includes profiling_config.h ahead of that conditional rather than receiving the levels through runtime_types.h: an #if on an undefined macro evaluates to 0, so a profiling block that reaches its levels transitively switches itself off silently the day that path changes. Four kinds of header that a tooling pass reads as unused are kept, each for a reason the tool cannot see: profiling_config.h supplies the macros the #if conditions test, so including it under one of them would evaluate to 0 and silently disable the feature; runtime_core.h in scheduler_cold_path.cpp supplies the complete RuntimeContext that rt->scheduler needs, where the forward declaration the tool finds first is not enough; and aicore.h, aicore_profiling_state.h and the two pto async .hpp files supply AICore builtins a host-side parser cannot see. Verified by building all four profiling configurations -- default, SIMPLER_ORCH_PROFILING, SIMPLER_SCHED_PROFILING and SIMPLER_DFX=0 -- across both architectures and both platform variants, since a header that only matters to a conditional block is invisible to a single build.
ChaoZheng109
approved these changes
Sep 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Every translation unit and header under
host_build_graphnow includes what it uses and nothing else, so a reader can tell a file's dependencies from its include block instead of following a chain of headers that happened to carry something along.105 includes removed, 51 added — a net 54 fewer — where the using file had been relying on a transitive path.
Three headers had become unofficial hubs
runtime_types.hcarriedruntime_status.handaicore_completion_mailbox.h;common.hcarriedassert_compat.h; andruntime.hcarried<stdio.h>and<string.h>under comments that named symbols — "for fprintf, printf", "for memset" —runtime.hitself never uses. None of the three used what it carried; consumers did, without naming it. All are gone, and the files reachingSIMPLER_ERROR_*,always_assert,debug_assertandstd::memsetthrough them now include those headers directly.Same shape at smaller scale:
tensormap.htakestensor_create_info.h,async_wait.htakesaicore_completion_mailbox_types.h,runtime_maker.cpptakeshost_phase_kind.h,sdma_completion_scheduler.htakesconstants.handcache_maintenance.h.One include moves inside its conditional, one moves ahead of it
scheduler.h'saicpu/device_time.h: all eight uses ofget_sys_cnt_aicpusit insideSIMPLER_*_PROFILINGblocks, so an unconditional include claimed a dependency the file only has when timing is compiled in. Its comment also claimed early-dispatch doorbell timing used it, which no call site does.The same file now includes
profiling_config.hahead of that conditional rather than receiving the levels throughruntime_types.h. The order happens to work today, but an#ifon an undefined macro evaluates to 0 — so a profiling block that reaches its levels transitively switches itself off silently the day someone tidies the header it came through. That is the failure mode the table below exists to avoid, and the file was subject to it.Four kinds kept despite the tooling saying otherwise
A pass with clang-tidy's
misc-include-cleanerproduced the candidate list, but it reads several correct includes as unused. Each of these was confirmed by deleting it and watching the build:profiling_config.h×5#ifconditions test — putting it inside one would make the condition evaluate to 0 and silently disable the featureruntime_core.hinscheduler_cold_path.cpp×2rt->schedulerneeds the completeRuntimeContext; the tool finds a forward declaration first and stopsaicore.h+aicore_profiling_state.hread_reg,get_physical_core_id, ...) a host-side parser cannot resolveasync .hpppto::comm::sdma::detail::*behind__ubuf__typesWhy four build configurations
A header that only matters inside
#if SIMPLER_DFXor a profiling block is invisible to a single build — deleting it passes, and the breakage only appears when someone turns that flag on. So every batch was checked against default,SIMPLER_ORCH_PROFILING,SIMPLER_SCHED_PROFILINGandSIMPLER_DFX=0, across both architectures and both platform variants.That is also why "the build passed" was not treated as evidence of completeness: a missed include leaves the build green by definition. Completeness came from re-running the scan until it reported nothing but the known exceptions.
Test
tensormap_and_ringbufferrebuilt on all 4 platforms to confirm the shared headers are unaffectedctest: 128/128.so, which theorchestration_api.hchanges reachRebased onto #2090, which rewrote
a5'saicpu_executor.cpp. Every include this PR had removed from that file was re-checked against the new code:<cinttypes>(PRIu64),aicpu/device_time.h,chip_swimlane_collector_aicpu.h,platform_regs.handplatform_config.hare now genuinely used and stay;<unistd.h>,<algorithm>,sys/mman.h,args_dump_aicpu.handpmu_collector_aicpu.hare not and are removed.Split out of #2098, which keeps the two behavioural changes it was mixed with.